Enable the retry for cert for handling InvalidCertificate error - #614
Enable the retry for cert for handling InvalidCertificate error#614SivaParvathip21 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the DDI MBOR integration-test helpers to make certificate retrieval more resilient to transient InvalidCertificate failures that can occur when a device soft-reset (iDFU) overlaps with DDI operations.
Changes:
- Added retry helpers for
GetCertificateto reattempt onDdiStatus::InvalidCertificatewith a fixed backoff. - Switched leaf-cert retrieval and cert-chain enumeration paths to optionally use the new retry logic.
- Applied the retry behavior to POTAs endorsement helper certificate retrieval.
Comments suppressed due to low confidence (3)
ddi/mbor/types/tests/integration/common.rs:238
- This retry path uses
println!, which produces unstructured stdout and is inconsistent with the surroundingtracing::*usage. Prefertracing::debug!/warn!for consistent, filterable logs.
println!("Retrying the get_cert operation for cert_id {}", cert_id);
ddi/mbor/types/tests/integration/common.rs:351
- The retry behavior in
helper_verify_leaf_certis also gated behindIDFU=1, which meansInvalidCertificateflakes won’t be handled unless the environment variable is set. Since the retry helper only delays on the specific transient error, using it unconditionally is typically safer for integration tests.
let idfu_enabled = std::env::var("IDFU").map(|v| v == "1").unwrap_or(false);
let result = helper_get_cert_chain_info(dev);
assert!(result.is_ok(), "result {:?}", result);
let resp = result.unwrap();
ddi/mbor/types/tests/integration/common.rs:387
helper_get_pota_endorsementhas the sameIDFU=1gating for retries. IfInvalidCertificatecan occur during concurrent soft reset, tests calling this helper will still flake unless that env var is set. Consider always using the retry helper and only sleeping when the transient error actually happens.
let idfu_enabled = std::env::var("IDFU").map(|v| v == "1").unwrap_or(false);
let get_cert_chain_info = helper_get_cert_chain_info(dev).unwrap();
let leaf_cert_id = get_cert_chain_info.data.num_certs - 1;
let cert_resp = if idfu_enabled {
tracing::debug!("Device is in IDfu mode");
| let cert_info = helper_get_cert_chain_info(dev); | ||
| assert!(cert_info.is_ok(), "cert_info {:?}", cert_info); | ||
| let resp = cert_info.unwrap(); | ||
| let num_certs = resp.data.num_certs; |
| if start.elapsed() > retry_window { | ||
| break; | ||
| } | ||
| println!("Retrying the get_cert operation"); |
| let idfu_enabled = std::env::var("IDFU").map(|v| v == "1").unwrap_or(false); | ||
|
|
||
| let resp = if idfu_enabled { | ||
| tracing::debug!("Device is in IDfu mode"); | ||
| helper_get_cert_with_retry(dev, 5).unwrap() | ||
| } else { | ||
| tracing::debug!("Device is not in IDfu mode"); | ||
| let result = helper_get_cert_chain_info(dev); | ||
| assert!(result.is_ok(), "result {:?}", result); | ||
| let chain_resp = result.unwrap(); | ||
| let num_certs = chain_resp.data.num_certs; | ||
| let result = helper_get_certificate(dev, num_certs - 1); | ||
| assert!(result.is_ok(), "result {:?}", result); | ||
| result.unwrap() | ||
| }; |
| // let result: Result<DdiGetCertificateCmdResp, DdiError> = helper_get_certificate(dev, i); | ||
| // assert!(result.is_ok(), "result {:?}", result); | ||
|
|
||
| // let resp = result.unwrap(); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
ddi/mbor/types/tests/integration/common.rs:200
- The docstring says this helper returns the last success/error, but it currently panics on
GetCertChainInfofailures due toassert!/unwrap(). Since the function already returnsResult, propagate the error instead (and avoid re-fetching the cert chain info on every retry when only the leaf cert id is needed).
let cert_info = helper_get_cert_chain_info(dev);
assert!(cert_info.is_ok(), "cert_info {:?}", cert_info);
let resp = cert_info.unwrap();
let num_certs = resp.data.num_certs;
ddi/mbor/types/tests/integration/common.rs:208
- Using
println!in retry loops can spam CI output and bypass the test logging configuration. Prefertracing(already used elsewhere in this file) and the existingstd::threadimport.
println!("Retrying the get_cert operation");
std::thread::sleep(std::time::Duration::from_millis(50));
ddi/mbor/types/tests/integration/common.rs:239
- Using
println!here can generate noisy test output on transient retries; prefertracingso it can be filtered/collected consistently.
println!("Retrying the get_cert operation for cert_id {}", cert_id);
std::thread::sleep(std::time::Duration::from_millis(50));
ddi/mbor/types/tests/integration/common.rs:368
- Remove the commented-out old implementation to keep the helper easier to read and maintain.
// let result: Result<DdiGetCertificateCmdResp, DdiError> = helper_get_certificate(dev, i);
// assert!(result.is_ok(), "result {:?}", result);
// let resp = result.unwrap();
ddi/mbor/types/tests/integration/common.rs:252
- The tracing message uses inconsistent capitalization ("IDfu") compared to the docstring’s "iDFU". Standardizing improves log searchability; please update the other occurrences in this file too.
tracing::debug!("Device is in IDfu mode");
|
@SivaParvathip21 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (5)
ddi/mbor/types/tests/integration/common.rs:368
- Please remove the commented-out, dead code block—it's no longer used and adds noise to the test helper.
// let result: Result<DdiGetCertificateCmdResp, DdiError> = helper_get_certificate(dev, i);
// assert!(result.is_ok(), "result {:?}", result);
// let resp = result.unwrap();
ddi/mbor/types/tests/integration/common.rs:358
- The debug message uses "IDfu" but the doc comments use "iDFU". Standardize the spelling/casing so logs and comments refer to the same mode.
let resp = if idfu_enabled {
tracing::debug!("Device is in IDfu mode, retrying get_certificate for cert {}", i);
helper_get_cert_by_id_with_retry(dev, i, 15).unwrap()
ddi/mbor/types/tests/integration/common.rs:391
- The debug messages use "IDfu" but the doc comments use "iDFU". Standardize the spelling/casing so logs and comments refer to the same mode.
tracing::debug!("Device is in IDfu mode");
helper_get_cert_by_id_with_retry(dev, leaf_cert_id, 15).unwrap()
} else {
tracing::debug!("Device is not in IDfu mode");
helper_get_certificate(dev, get_cert_chain_info.data.num_certs - 1).unwrap()
ddi/mbor/types/tests/integration/common.rs:188
helper_get_cert_with_retryusesAzihsmDdidirectly for the device type, while the rest of this integration test module consistently uses theDdiTestalias. Using the alias here improves consistency and avoids confusion when the alias changes.
fn helper_get_cert_with_retry(
dev: &<AzihsmDdi as Ddi>::Dev,
retry_secs: u64,
) -> Result<DdiGetCertificateCmdResp, DdiError> {
ddi/mbor/types/tests/integration/common.rs:255
- The debug messages use "IDfu" but the doc comments use "iDFU". Standardize the spelling/casing so logs and comments refer to the same mode.
This issue also appears in the following locations of the same file:
- line 356
- line 387
tracing::debug!("Device is in IDfu mode");
helper_get_cert_with_retry(dev, 15).unwrap()
} else {
tracing::debug!("Device is not in IDfu mode");
Enable the get_cert_retry for handling the InvalidCertificate issue while device goes for a softreset concurrently with DDI Operation.